Skip to content

Version 1#5

Open
Evaki wants to merge 2 commits into
GlobalWebIndex:mainfrom
Evaki:version-1
Open

Version 1#5
Evaki wants to merge 2 commits into
GlobalWebIndex:mainfrom
Evaki:version-1

Conversation

@Evaki
Copy link
Copy Markdown

@Evaki Evaki commented Sep 4, 2025

This is my attempt for a solution to the challenge. I do not have any experience in go but focused on creating a tidyish structure with the go api. As for the python bit it was created fast and is a bit untidy and some bits do not work. Given the tight timeframe I think I did what I could. Any feedback is, of course, welcome. Thank you!

Copy link
Copy Markdown

@alexandros-georgantas alexandros-georgantas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @Evaki 👋

Thank you for your work on this assignment!

Overall, your submission addresses some aspects of the task, though the solutions lean towards the simpler side. I sensed that our deadline might have been tight for you, and perhaps with more time, the outcome would have been different.

It's perfectly fine that some elements were missed.

I've left some comments on your Pull Request with questions. I'd be happy to hear your answers if you're inclined to provide them.

Thanks,
Alex

Comment thread mongo-init.js
@@ -0,0 +1,33 @@
db.createUser(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given more time, how would you have refactored this schema?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

I would have probably added stricter validation rules and indexing like so:

  • add pattern validation for uuids in session_id field which will ensure uniqueness of ids and can be easily generated using libraries (most languages provide such libraries)

  • add validation for the messages array structure:

    • set an enum for the role field enum: ["question", "answer"]
  • add a maxItems field to the messages array, so that there are no errors or performance issues produced from large arrays (as this application might only need to store a limited number of recent messages for a session I would set it to 100)

  • add min and max length for text (minLength: 1, maxLength: 1000) which would prevents the creation of excessively large strings that could impact performance or storage

  • add indexing by sessionId for faster search performance (db.userhistory.createIndex({ sessionId: 1 });)

@@ -0,0 +1,147 @@
package controller
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please describe what would have been your approach in order to test the functionality of this file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(First of all, I believe that the function that calls the other API should be moved to a different file and not sit in the controller file)

As a first approach, I would use mock libraries to mock the calls to the python API and the database operations (possibly this one: github.com/stretchr/testify/mock). That way, I can test the controller logic independently of dependencies. The test cases would roughly cover the following:

  • request validation
  • errors from request validations
  • error handling from database errors
  • error handling from empty query response from database
  • error handling from python api call errors
  • error handling of empty response from python API
  • empty message inputs
  • very long message inputs
  • unusual messages (check user input is properly sanitizes)

Comment thread src/api/model/model.go
@@ -0,0 +1,29 @@
package model
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you had time, what other entities you would have created to meet the requirements of the assignment (e.g. feedback aspect)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements of the assignment, based on the readme file and my understanding, was to create chat bot endpoint/web service that:

  • uses the data provided to give users replies (DONE - python API)
  • persists the chat data (DONE - stored in mongo db)
  • continue the conversation where it was left off
  • user opens multiple chats (DONE - use of session Ids)

For the third bullet point I would have created an endpoint that returns the user's conversation.

As for the optional enhancements which included the feedback aspect, I would have used the same post endpoint and store the feedback in the database in the messages list with an extra role called "feedback" that would have a positive or negative value and would include an optional comment from the user.

fmt.Println(uri)

// Initialize global cache
GlobalCache, err = bigcache.NewBigCache(bigcache.DefaultConfig(30 * time.Minute))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've initialized a cache. How is it being utilized within your code? Please explain your decision to include a caching layer and its intended use.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure for my go app was derived from Rohit Binjola's example app (the link can be found in the top of the main.go app). They added the cache capability to their database module. I did not end up using it, but I could possibly use it in the database handler if I were, for example, to fetch the session data from the database based on specific session ids.

Comment thread src/router/router.go
resource := router.Group("/api")
resource.Use(middleware.LogRequestInfo())
{
resource.POST("/question", controller.SubmitQuestion)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you had more time what other endpoints you would have created to meet the assignment's requirements?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer: #5 (comment)

Comment thread src/router/router.go

func NewRouter() *gin.Engine {
router := gin.New()
resource := router.Group("/api")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please provide a suggestion on how this could be refactored to support potential breaking changes of the API?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use versioning by creating different paths /api/vi, where i is the version of the api. That will allow us to change the behavior or structure of your API without affecting users.

Copy link
Copy Markdown

@dzacharakis dzacharakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for your submission!
We really appreciate the time and effort you put into this. I have a few questions I’d love your thoughts on—could you take a look when you get a chance?

Thanks

Comment thread README.md
### 🐳 Service Description
#### 1. Question API (Go)

Port: 8080
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: config and compose expose 7004


response, err := PostJSON[RequestPayload, model.SubmitQuestionRequest](url, req)
if err != nil {
log.Fatalf("Fatal error calling matching API: %v", err)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t it be better to use log.Errorf instead and why?

Score float64 `json:"score"`
}

func RemoveConversation(c *gin.Context) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that RemoveConversation exists but isn’t registered in the router. Leftover?

Comment thread src/router/router.go

func Init() {
router := NewRouter()
router.Run(config.Appconfig.GetString("server.port"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error return value of router.Run is not checked.

Suggested change
router.Run(config.Appconfig.GetString("server.port"))
err := router.Run(config.Appconfig.GetString("server.port"))
if err != nil {
log.Fatalf("Failed to start server: %v", err)
}

Any possible tool that could be helpful here?

Comment thread src/router/router.go
}

func NewRouter() *gin.Engine {
router := gin.New()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting the server would crash on an internal error. If you want it to keep running as expected, what approaches could you take? Two come to my mind—what might Gin offer that you may have missed?

Comment thread src/go.sum
rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
rsc.io/pdf v0.1.1/go.mod h1:n8OzWcQ6Sp37PL01nO98y4iUCRdTGarVfzxY20ICaU4=
rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0=
rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this go.sum file has 650 lines of packages. Have you considered module maintenance—like cleaning up/removing unused modules?

req.Header.Set("Content-Type", "application/json")

// Send the request
resp, err := client.Do(req)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to also check resp.StatusCode here before JSON unmarshal?

}

// PostJSON sends a POST request with JSON-encoded data and decodes the JSON response.
func PostJSON[T any, R any](url string, payload model.SubmitQuestionRequest) (*ResponsePayload, error) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are generics used here if neither type parameter (T, R) is actually utilized? As it stands, this looks like a normal function with unused type parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants